-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: public commands API #232
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimal change to make AddCommand public, looks good to me with an extremely minor comment.
Logical follow-up step on #226, looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the last change required on the Pebble side before we can create a different variant of Pebble in a separate repository.
Some work remains to allow for personality changes to update string references to "pebble" in the help, but that will be a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly okay since it's a private use, but I still hope we can improve it slightly as outlined below. Bonus points if there's a test for it.
* AddHelpCategory() was removed in favor of an exported HelpCategories global. * AddCommand() now accepts a CmdInfo struct as its sole argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving it forward. There are still remaining issues related to the points made in the prior review it seems.
* Removed unused struct members such as Aliases and Hidden. * Replace the Extra function with PassAfterNonOption, as this is the only internal flags.Command option that justified its need. * Rename OptDesc/ArgDesc for clarity. * Document the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some subjective preferences, but otherwise looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few extra details, but it looks like the points discussed last time about debug commands still haven't been touched?
The following flags will pass: `-a`, `--long-flag`, `--b`, `--this-is-a-flag`; the following flags won't: `---`, `---c`, `--quasi--valid`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Angel. One more pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work, Angel. Hopefully this is ready to go in now.
Introduce a public API for well-known Pebble-based applications to use.
This PR introduces two basic changes on top of those included in #226:
cli.AddCommand()
function, so that applications can add new commands.cli.HelpCategories
variable, so that applications can add new sections to the help manual.